-
Notifications
You must be signed in to change notification settings - Fork 49
Reenable incremental builds #162
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Reenable incremental builds #162
Conversation
MatthewHambley
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some improvements possible to structure and code. Also some lack of clarity regarding why certain things are done in certain ways.
| commands = ( | ||
| f"git -C {loc} init", | ||
| f"git -C {loc} remote add origin {repo_source}", | ||
| f"git -C {loc} fetch origin {repo_ref}", | ||
| f"git -C {loc} checkout FETCH_HEAD", | ||
| f"git -C {loc} fetch origin main:main", | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems a lot of effort. Doesn't it just recreate a clone?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it was initially written to see if we could save space while cloning sources for use in rose-stem. It turns out the difference is very small, but as this was marginally smaller, I left it in. Given it's copied from SimSys_Scripts I don't think it's worth modifying here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case a comment saying that it is exactly equivalent to "clone" will be welcome by anyone trying to understand why all this is happening.
| # Fetch the main branch from origin | ||
| # Ignore errors - these are likely because the main branch already exists | ||
| # Instead write them as warnings | ||
| command = f"git -C {loc} fetch origin main:main" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you need to treat the copy as a repository, why not clone it. That's what distributed repositories are all about. You might take advantage of a sparse clone to gain the advantage of filtering you are getting from the rsync exclusion list.
…mo/lfric_apps into improve_local_builds
james-bruten-mo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Matthew - those comments are addressed, and for the most part changes done
| commands = ( | ||
| f"git -C {loc} init", | ||
| f"git -C {loc} remote add origin {repo_source}", | ||
| f"git -C {loc} fetch origin {repo_ref}", | ||
| f"git -C {loc} checkout FETCH_HEAD", | ||
| f"git -C {loc} fetch origin main:main", | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it was initially written to see if we could save space while cloning sources for use in rose-stem. It turns out the difference is very small, but as this was marginally smaller, I left it in. Given it's copied from SimSys_Scripts I don't think it's worth modifying here
MatthewHambley
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few outstanding conversations and a few new thoughts.
| commands = ( | ||
| f"git -C {loc} init", | ||
| f"git -C {loc} remote add origin {repo_source}", | ||
| f"git -C {loc} fetch origin {repo_ref}", | ||
| f"git -C {loc} checkout FETCH_HEAD", | ||
| f"git -C {loc} fetch origin main:main", | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case a comment saying that it is exactly equivalent to "clone" will be welcome by anyone trying to understand why all this is happening.
Co-authored-by: Matthew Hambley <MatthewHambley@users.noreply.github.com>
PR Summary
Sci/Tech Reviewer: @MatthewHambley
Code Reviewer: @t00sa
At the git migration incremental builds on the command line no longer worked as timestamps in github are based on clone date, not commit date. This reenables this functionality by fetching/pulling changes if the build is being rerun. This has been tested on the command line for both a run with physics code (lfric_atm) and without (gungho_model). The 2nd build time is much quicker as expected.
This also enables building using the local github mirrors which will be useful once these are also available on the HPC. Again, these have been tested on the cli and with incremental builds.
This change adds
get_git_sourceswhich is mostly a copy of the same file from SimSys_Scripts. Having 2 copies isn't ideal, but I can't think of another way to make that SimSys_Scripts file available when building locally (short of installing it as a library). Hopefully this is something that'll be improved by fab!Code Quality Checklist
Testing
trac.log
Test Suite Results - lfric_apps - test_incremental_builds_change/run2
Suite Information
Task Information
✅ succeeded tasks - 1106
Security Considerations
Performance Impact
AI Assistance and Attribution
Documentation
PSyclone Approval
Sci/Tech Review
(Please alert the code reviewer via a tag when you have approved the SR)
Code Review